-
Notifications
You must be signed in to change notification settings - Fork 456
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(lib): fix source path generated for local modules #947
fix(lib): fix source path generated for local modules #947
Conversation
aef0a7c
to
5ca1143
Compare
@JonCubed Very much appreciate the fix; however, at least longer term I think that local modules need to be treated like an asset (see #648). The reason being is so that they work when using Terraform Cloud. |
@jsteinich what is proposed for Assets was what I initial thought would happen with modules, so looks good to me long term. Short term I would appreciate if this could be accepted as use local modules is currently broken. |
ad1dd5e
to
f370739
Compare
Agree with you @JonCubed, I tested the TerraformAsset option and it didn't work @jsteinich because in the cdk.tf.json file, the synt command is puting an absolute path instead of a relative path and also you can't set a relative path on the TerraformAsset, it should be absolute.
I tried adding the "./" to the
I reviewed the cdk.tf.json file generated and I found I modified the cdk.tf.json file manually, changing it for None of the solutions you mention work @jsteinich, why don't you just accept the solution from @JonCubed ? |
I'm not sure the exact order of operations, but @DanielMSchmidt also started a PR for doing the "longer term" solution. Given the asset relative directory complications, I certainly agree with merging this approach. If the other PR finishes in time for the next release, great, but if not then the situation is still improved. @JonCubed apologizes for not responding sooner. I don't have the ability to merge, but I can certainly poke the rest of the team. |
no worries @jsteinich , I've worked around it by patching my local and docker image. I've played around with this some more and one thing that might make this fix undesirable is that it only works for the default outputs i.e. cdktf.out/stacks/. In order to make it work for any output folder structure we would need to get the value from the output flag or cdktf.json to determine how far back we need to go. I wasn't sure how I could get this value and if this is a deal breaker or not for the short term. |
@JonCubed Could you rebase this? |
f370739
to
f1abcc3
Compare
@DanielMSchmidt rebased |
f1abcc3
to
9814ef0
Compare
f5a5e4f
to
dfca7c5
Compare
@jsteinich @DanielMSchmidt I've rebased this again. Any chance we can merge this or co-ordinate so that this can be merged? |
@DanielMSchmidt thank you |
I'm going to lock this pull request because it has been closed for 30 days. This helps our maintainers find and focus on the active issues. If you've found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
This fixes issue #946